-
Notifications
You must be signed in to change notification settings - Fork 66
macOS support #40
base: master
Are you sure you want to change the base?
macOS support #40
Conversation
Compiles, but example segfaults due to an NSInvalidArgumentException, haven't had time to figure this out yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not check it in details (and did not test it as well), but apart from the things I've mentioned in comments it looks ok. Fixing the CI and/or PR to pass all checks would also be nice.
Cargo.toml
Outdated
objc-foundation = "*" | ||
objc_id = "*" | ||
cocoa = "*" | ||
core-foundation = "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to have some more strict version requirement for production usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 6db1648
src/api/cocoa/mod.rs
Outdated
use objc_id::Id; | ||
use crate::{Application, BoxedError, Callback, Error}; | ||
|
||
/// The generation representation of the Mac OS X application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A typo in a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 2d98b78
src/api/cocoa/mod.rs
Outdated
} | ||
|
||
unsafe impl Send for Window {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not seem right. Window
contains an Rc
which is clearly !Send
, so just declaring Window
to be Send
is not sound.
It seems like making Window
implement Send
is not the best idea, not only because it contains types which are not Send
, but also due to the fact that on OS X most of the things you can do with a window must be done from the main thread. The only way to really make it Send
is to use GCD inside each function the user may call on a Window
after moving it into a different thread, i.e. make sure that that all actions we apply to Window
are executed in a main thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in 3a00d0d
Thanks for mentioning those issues, I updated the PR fixing those and it passed all checks now. |
Awesome PR! I'm a little busy this week, but I'm hoping to take a look at this and possibly get it in and roll another package version this weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs visibility fix to compile.
With the visibility specifier fix, this will compile on all platforms.
- On linux (debian, exwm), trayicon example runs ok, exits ok.
- On macos, trayicon example runs ok, errors on exit with "illegal hardware instruction"
- On windows, trayicon example panics on start, "process didn't exit successfully"
If you can fix up the visibility, I'll try to get stacks for the mac and windows errors. RUST_BACKTRACE isn't working for some reason.
src/lib.rs
Outdated
} | ||
|
||
pub fn wait_for_message(&mut self) -> Result<(), Error> { | ||
#[cfg(not(target_os = "macos"))] | ||
fn wait_for_message(&mut self) -> Result<(), Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed the visibility specifier here, meaning the example won't build on linux or windows. Just need to add pub (and I really need to get CI going for all platforms).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a5cc6bc
Hi, are there remaining blockers before this could be merged? |
The pub on wait_for_message was fixed, but I believe this still panics on windows. So that's a blocker. |
Includes @application-developer-DA's patches and support for missing features, such as callbacks, separators and icons from files. Partially derived from https://github.com/rust-sysbar/rust-sysbar.